Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to make standalone run also from within a CMSSW area #36

Conversation

VourMa
Copy link
Collaborator

@VourMa VourMa commented Jun 8, 2024

When we are within a CMSSW area, we would like the data files to be picked up from the CMSSW_SEARCH_PATH. This was not possible with the current setup.sh, as it was resetting the search path. This behavior is now modified, partially according to #30 (comment), so it may be a controversial change and feedback is welcome.
That also means that we want to change the order with which we check the potential directories in which the data files may reside, and this is also taken care of in this PR.

Once we agree on the changes, I can upload a README with instructions on how to test the standalone both within and without a CMSSW area.

@VourMa
Copy link
Collaborator Author

VourMa commented Jun 10, 2024

@ariostas Please let me know what you think of this one. I would like to decide on whether this looks fine or not before writing the instructions for how to test PRs in CMSSW locally.

@ariostas
Copy link
Member

@VourMa The changes look good to me. It's up to you whether we should remove LST_BASE now or just do it later.

@ariostas
Copy link
Member

It shouldn't break the CI, but just to make sure.

/run all

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.1    325.1    123.1     47.6     95.5    503.5    132.4    158.1    100.1      1.8    1531.4     983.9+/- 256.0     415.1   explicit_cache[s=4] (target branch)
   avg     44.0    323.3    121.7     48.0     95.9    505.7    133.2    156.8     99.4      1.9    1529.7     980.0+/- 257.3     416.5   explicit_cache[s=4] (this PR)

@slava77
Copy link

slava77 commented Jun 10, 2024

I'm not sure this should be just in the devel branch; the updates are supposedly in the context of the cms-sw PR to get things run in standalone.

However, since the CI runs already, I guess this change can be applied later (no need to add this to the batch2 branch as well).
Is my understanding correct?

@VourMa
Copy link
Collaborator Author

VourMa commented Jun 10, 2024

It's up to you whether we should remove LST_BASE now

I can remove it now to be done with it. I will add one more commit to this PR a bit later today.

I'm not sure this should be just in the devel branch; the updates are supposedly in the context of the cms-sw PR to get things run in standalone.

This can definitely go in the batch 2 as well, and maybe it finalizes the "Modifications to the standalone scripts" to-do that we have set in the cms-sw PR description, although I am not sure of the best way to do it. Let me know what you prefer and I will do it.

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@VourMa
Copy link
Collaborator Author

VourMa commented Jun 11, 2024

@slava77 @ariostas If there are no more comments, please approve the PR but do not merge it yet - I would like to push the README with the instructions.

@VourMa
Copy link
Collaborator Author

VourMa commented Jun 11, 2024

I think this is ready to be merged now. People can test the README instructions to check whether they work for them.

Comment on lines +66 to +67
export PATH=/cvmfs/cms.cern.ch/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin:/cvmfs/cms.cern.ch/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_0_pre3/external/el8_amd64_gcc12/bin:$PATH
export LD_LIBRARY_PATH=/cvmfs/cms.cern.ch/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_0_pre3/biglib/el8_amd64_gcc12:/cvmfs/cms.cern.ch/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_0_pre3/lib/el8_amd64_gcc12:/cvmfs/cms.cern.ch/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_0_pre3/external/el8_amd64_gcc12/lib:/cvmfs/cms.cern.ch/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib64:/cvmfs/cms.cern.ch/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib:$LD_LIBRARY_PATH
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow how this can be a part of when CVMFS is not available; perhaps the text from below they can point to local directories can be added here instead?

  • Shouldn't only external/gcc and external/cuda be necessary instead of the cmssw path?
  • are the biglib and cmssw/lib really needed; I'd expect only gcc and cuda are needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that the extra library paths was because the gcc in cvmfs has some preconfigured stuff that depends on the other libraries, but in principle one would just need vanilla gcc and nvcc.

@VourMa
Copy link
Collaborator Author

VourMa commented Jun 12, 2024

General reply on the comments for the README: both are valid, and especially for the first one, I don't know how it worked before - this was just (blindly) keeping the relevant README section from what already existed. It is clear that it needs to be updated. Having said that, I am not willing to come up with all the details for the CVMFS-less configuration at this stage. My proposal is to comment out that part of the README and come back to it if needed in the future.

Currently, the data files for LST need to be copied manually. This is done by running:

```bash
git clone [email protected]:cms-data/RecoTracker-LSTCore.git data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth mentioning where this data directory should reside. It's not clear whether it's in standalone or LSTCore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and in fact I had the directory wrong in one case. I now fixed that and added an explicit mention of the directory where the data files should end up.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the setup.sh, after it's called the $CMSSW_SEARCH_PATH will already be set.
So, the same git clone [email protected]:cms-data/RecoTracker-LSTCore.git $CMSSW_BASE/external/$SCRAM_ARCH/data/RecoTracker/LSTCore/data should work (note that cd in and out of the target directory is not really necessary)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nevermind; that search path will not include the local CMSSW checkout

@slava77
Copy link

slava77 commented Jun 13, 2024

/run all

Copy link

There was a problem while building and running with CMSSW. The logs can be found here.

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     43.9    323.9    122.7     48.2     95.9    500.3    134.0    156.8    105.8      1.8    1533.2     989.0+/- 259.8     418.0   explicit_cache[s=4] (target branch)
   avg     43.7    325.7    123.4     50.2     97.4    499.9    133.9    158.3    106.7      1.6    1541.0     997.4+/- 263.0     420.8   explicit_cache[s=4] (this PR)

@slava77
Copy link

slava77 commented Jun 13, 2024

There was a problem while building and running with CMSSW. The logs can be found here.

@ariostas is there a problem in the CI ?
It looks like it failed at checking out the baseline/reference part

@ariostas
Copy link
Member

is there a problem in the CI ?

Sorry, there were two steps that should have been done in the opposite order. I'm re-running it now.

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@slava77 slava77 merged commit 6c466f1 into CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel Jun 14, 2024
3 checks passed
@ariostas ariostas mentioned this pull request Nov 6, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants